Handle try in Flatten pass #2567

aheejin · 2020-01-03T04:56:12Z

This adds handling of try in the Flatten pass. This is in a way similar
to 'if' handling.

aheejin · 2020-01-06T01:37:31Z

This turned out not working in some cases yet; I'm working on a fix.

This is to remove a code pattern that generates an additional `unreachable` after an unreachable expression. This happens when the unreachable expression is a direct child of a block/loop/try. Block/loop/try preserves its childrens' preludes within it, which means childrens' preludes are not gonna escape the structure. So this kind of code pattern is often generated by Flatten: ```wast (block (some unreachable expression) (unreachable) ;; unnecessary ) ``` This PR removes the unnecessary `unreachable`s generated. On the other hand, `if` does not satisfy that property because `if`'s condition's prelude can escape `if`. This is not for optimization (Flatten is not for optimization per se; it's more of an enabler of other optimization passes, so we don't need to optimize the code generated by Flatten because we have other passes after this); This is done in order to make for WebAssembly#2567 to work. (We are planning to disable Flatten for exceptions now, but that's because of `br_on_exn`; try handling part (WebAssembly#2567) can land now.)

This is to remove a code pattern that generates an additional `unreachable` after an unreachable expression. This happens when the unreachable expression is a direct child of a block/loop/try. Block/loop/try preserves its childrens' preludes within it, which means childrens' preludes are not gonna escape the structure. So this kind of code pattern is often generated by Flatten: ```wast (block (some unreachable expression) (unreachable) ;; unnecessary ) ``` This PR removes the unnecessary `unreachable`s generated. On the other hand, `if` does not satisfy that property because `if`'s condition's prelude can escape `if`. This is not for optimization (Flatten is not for optimization per se; it's more of an enabler of other optimization passes, so we don't need to optimize the code generated by Flatten because we have other passes after this); This is done in order to make for WebAssembly#2567 to work. We are planning to disable Flatten for exceptions now, but that's because of `br_on_exn`; try handling part (WebAssembly#2567) can land now. Current Flatten generates unnecessary `block`s because it generates unnecessary `unreachable`s and wrap the original unreachable expression and the new `unreachable` instruction within a `block`, ending up generating an extra `block`s within `catch`, which is a problem.

This is to remove a code pattern that generates an additional `unreachable` after an unreachable expression. This happens when the unreachable expression is a direct child of a block/loop/try. Block/loop/try preserves its childrens' preludes within it, which means childrens' preludes are not gonna escape the structure. So this kind of code pattern is often generated by Flatten: ```wast (block (some unreachable expression) (unreachable) ;; unnecessary ) ``` This PR removes the unnecessary `unreachable`s generated. On the other hand, `if` does not satisfy that property because `if`'s condition's prelude can escape `if`. This is not for optimization (Flatten is not for optimization per se; it's more of an enabler of other optimization passes, so we don't need to optimize the code generated by Flatten because we have other passes after this); This is done in order to make for WebAssembly#2567 to work. We are planning to disable Flatten for exceptions now, but that's because of `br_on_exn`; try handling part (WebAssembly#2567) can land now. Current Flatten generates unnecessary `block`s because it generates unnecessary `unreachable`s and wrap the original unreachable expression and the new `unreachable` instruction within a `block`, ending up generating an extra `block`s within `catch`, which is a problem, because this can violate the invariant that `exnref.pop` should follow after `catch`.

This adds handling of try in the Flatten pass. This is in a way similar to 'if' handling.

aheejin · 2020-01-11T01:20:13Z

This is ready for review. This has to land after #2583. In order to be correct, the tests outputs are generated with both #2583 and this PR, but for ease of review this PR does not contain #2583's Flatten code change. (So the CI will not pass) I'll merge master after #2583 lands.

aheejin · 2020-01-13T23:26:30Z

I'll hold this for the moment, because we discovered a problem in #2583, the prerequisite for this.

aheejin · 2021-11-22T19:59:03Z

I updated this old PR to use the new EH spec. Now we can use #4348 and this is not dependent on #2583 anymore.

kripken · 2021-11-22T21:17:41Z

src/passes/Flatten.cpp

+
+    // Flatten can generate blocks within 'catch', making pops invalid. Fix them
+    // up.
+    EHUtils::handleBlockNestedPops(curr, *getModule());


It looks like handleBlockNestedPops will scan for Trys without checking the features first. How about adding an early exit there if exceptions are not enabled?

Done at the beginning of handleBlockNestedPops.

kripken · 2021-11-22T21:28:17Z

src/passes/Flatten.cpp

+        Expression* prelude = nullptr;
+        if (type.isConcrete()) {
+          Index temp = builder.addVar(getFunction(), type);
+          if (tryy->body->type.isConcrete()) {


I think this if just skips adding a set if the body is unreachable, as a minor optimization? I don't think it's needed IIUC. And the catch bodies do not do this check which makes it a little surprising to see it here.

I copy-pasted if's code here again:

binaryen/src/passes/Flatten.cpp

Lines 152 to 159 in 3c1c8ae

if (type.isConcrete()) {

Index temp = builder.addVar(getFunction(), type);

if (iff->ifTrue->type.isConcrete()) {

iff->ifTrue = builder.makeLocalSet(temp, iff->ifTrue);

}

if (iff->ifFalse && iff->ifFalse->type.isConcrete()) {

iff->ifFalse = builder.makeLocalSet(temp, iff->ifFalse);

}

I tried to remove these if (iff->ifTrue->type.isConcrete()) and if (iff->ifFalse && iff->ifFalse->type.isConcrete()) to see if these also can be removed, but if I do that these test fail:

Failed Tests (2): Binaryen lit tests :: passes/flatten_simplify-locals-nonesting_souperify-single-use_enable-threads.wast Binaryen lit tests :: passes/flatten_simplify-locals-nonesting_souperify_enable-threads.wast

I haven't checked why they fail (and I have to run for today), but I guess there can be cases these are necessary...? If that's the case I think I should also check the catch bodies too. I'll do that for the moment, and if they turn out not to be necessary, I will remove them.

I see, thanks. In that case let's check the catch bodies too, that seems most consistent with the rest of the code.

I think it's OK to remove these body checks (both in if and try); the reason those tests failed was not because they crashed, but because the filecheck results were different. As you said, this does a small optimization that prevents this form:

(local.set $x (unreachable) )

But I think it's OK to leave them (both in if and try) anyway; while Flatten doesn't try to do any optimizations itself, it wouldn't hurt, and maybe the output will be a little easier to read...

sgtm, this simple pattern doesn't add much code complexity and it does make the output smaller and simpler.

kripken · 2021-11-22T21:31:01Z

src/passes/Flatten.cpp

+        }
+        tryy->finalize();
+        if (prelude) {
+          ReFinalizeNode().visit(prelude);


if prelude is not null, isn't it equal to tryy? If so then we finalized it two lines above?

Hmm, I think you're right. I kind of copy-pasted if's code... Should I fix if too?

By the way, are these any different?

expr->finalize();

vs.

ReFinalize().visit(expr);

Ah, let's fix if too then, looks like it's also redundant there. Probably a copy-paste error there from back in history...

Yes, those two expressions are identical in practice, but the first uses static type info to call the right finalize function while the second is dynamic.

Removed the second (redundant) finalization from both try and if.

Hmm, turns out if's finalization was necessary, because if has condition so prelude can be different from iff. Restored if's finalization.

test/lit/passes/flatten-eh.wast

kripken

lgtm % comments

kripken · 2021-11-22T22:03:40Z

src/passes/Flatten.cpp

+        Expression* prelude = nullptr;
+        if (type.isConcrete()) {
+          Index temp = builder.addVar(getFunction(), type);
+          if (tryy->body->type.isConcrete()) {


I see, thanks. In that case let's check the catch bodies too, that seems most consistent with the rest of the code.

kripken · 2021-11-22T22:04:42Z

src/passes/Flatten.cpp

+        }
+        tryy->finalize();
+        if (prelude) {
+          ReFinalizeNode().visit(prelude);


Ah, let's fix if too then, looks like it's also redundant there. Probably a copy-paste error there from back in history...

Yes, those two expressions are identical in practice, but the first uses static type info to call the right finalize function while the second is dynamic.

kripken · 2021-11-22T22:07:17Z

Also please fuzz before landing.

kripken

Nice!

kripken · 2021-11-23T16:11:16Z

src/passes/Flatten.cpp

+        Expression* prelude = nullptr;
+        if (type.isConcrete()) {
+          Index temp = builder.addVar(getFunction(), type);
+          if (tryy->body->type.isConcrete()) {


sgtm, this simple pattern doesn't add much code complexity and it does make the output smaller and simpler.

aheejin · 2021-11-29T18:52:30Z

As we talked last week, the EH fuzzing is not really working atm, and I'm working on a new PR that fuzzes EH (based on the initial contents), but it keeps finding new bugs now unrelated to Flatten. I'm not sure if holding off landing this until the fuzzer runs long enough is worth it; this doesn't add a new regression at least, because this feature has not been working anyway. How about landing this first?

kripken · 2021-11-29T19:37:43Z

Landing this now sgtm if it won't cause fuzz errors on main, which I guess it won't as EH fuzzing is still off as you said?

aheejin · 2021-11-29T20:25:47Z

Yes

aheejin requested a review from kripken January 3, 2020 04:56

aheejin mentioned this pull request Jan 11, 2020

Don't generate redundant unreachables in Flatten #2583

Closed

aheejin force-pushed the flatten_try branch from d3b0d63 to f629369 Compare January 11, 2020 01:17

Handle try in Flatten pass

fdbbd74

This adds handling of try in the Flatten pass. This is in a way similar to 'if' handling.

aheejin force-pushed the flatten_try branch from f629369 to fdbbd74 Compare January 11, 2020 01:19

Base automatically changed from master to main January 19, 2021 21:59

aheejin added 4 commits November 21, 2021 21:34

Merge branch 'main' into flatten_try

834155a

New spec + test

d58252e

Merge branch 'main' into flatten_try

574121c

Filecheck output

8fb0c89

kripken reviewed Nov 22, 2021

View reviewed changes

Address comments

111175b

kripken approved these changes Nov 22, 2021

View reviewed changes

aheejin added 3 commits November 22, 2021 22:11

Remove redundant finalization from if/try

503d2bb

Fix try + restore if's finalization

3725b1b

Add unreachable tests

d1e6f9b

kripken approved these changes Nov 23, 2021

View reviewed changes

aheejin merged commit 5f6911f into WebAssembly:main Nov 29, 2021

aheejin deleted the flatten_try branch November 29, 2021 21:57

	if (type.isConcrete()) {
	Index temp = builder.addVar(getFunction(), type);
	if (iff->ifTrue->type.isConcrete()) {
	iff->ifTrue = builder.makeLocalSet(temp, iff->ifTrue);
	}
	if (iff->ifFalse && iff->ifFalse->type.isConcrete()) {
	iff->ifFalse = builder.makeLocalSet(temp, iff->ifFalse);
	}

Handle try in Flatten pass #2567

Handle try in Flatten pass #2567

Uh oh!

Conversation

aheejin commented Jan 3, 2020

Uh oh!

aheejin commented Jan 6, 2020

Uh oh!

aheejin commented Jan 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aheejin commented Jan 13, 2020

Uh oh!

aheejin commented Nov 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Nov 22, 2021

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aheejin commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kripken commented Nov 29, 2021

Uh oh!

aheejin commented Nov 29, 2021

Uh oh!

Uh oh!

aheejin commented Jan 11, 2020 •

edited

Loading

aheejin commented Nov 29, 2021 •

edited

Loading